-
Notifications
You must be signed in to change notification settings - Fork 107
fix(datasets): Fix StudyDataset to properly propagate RDB password
#1077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Guillaume Tauzin <[email protected]>
ca8b961 to
e827ed9
Compare
| pruner_config = load_args.pop("pruner") | ||
| pruner = self._get_pruner(pruner_config) | ||
|
|
||
| storage_url_str = self._storage_url.render_as_string(hide_password=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this expose your database password? I have never used Optuna, so don't really know how this works, but ideally this should still be a secure way of handling credentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does indeed expose your database password. I am not sure what are the security consequences of this but I am not seeing a workaround.
Optuna's RDBStorage, requires the url of the RDB in the form of a SQLAlchemy URL string and stores it directly as a class attribute. This string includes the target db, its host, port, password and name. Previously with self._storage = str(storage), the password was hidden and translated to "***" within the string which meant authentication to a password-protected RDB was impossible. Now even if I instantiate the RDBStorage class in the dataset class constructor to reuse it in save/load methods, its url class attribute would still contain all the credentials.
Would you have any pointers regarding to the security scenario where that would be problematic or to a discussion regarding secure handling of credentials? I am happy to learn more about this and try and come back with a better solution!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @merelcht
It's been a few months already and I would really like to get this merge as the current StudyDataset does not really allow for distributed hyperparameter tuning. I am happy to work on a solution but I need some pointers as mentioned above. Please let me know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gtauzin , sincere apologies for not getting back to you earlier! This slipped my mind during and after the 1.0 release of Kedro.
My main concern is whether this could accidentally leak credentials and make it possible for bad actors to retrieve your database keys. Would it work if you just directly call:
storage = optuna.storages.RDBStorage(url=self._storage_url)
And skip the rendering as string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with the password handling and don't necessarily see something better. The rendered password is only in a variable, not in anything saved to the class, right? Unless I'm missing something in my pass through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepyaman Indeed, all credentials are stored in a class variable _storage_url which means they can be directly accessed as my_dataset._storage_url.render_as_string(hide_password=False). This is the only place where they appear.
@merelcht No worries! Initially I figured you were busy with 1.0 and then I forgot about it until I started needing it again :)
For your suggestion, the url parameter required by RDBStorage is a string, which is why I use the render_as_string method.
I have had a look at redis.PickleDataset and it seems to me you can get back credentials from a dataset instance as well by doing my_pickle_dataset._redis_db.connection_pool.connection_kwargs.
Is the problem related to credentials are stored in the class and can be accessed clearly or the presence of a line of code which exposes them clearly?
Thanks to both of you for the help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have had a look at redis.PickleDataset and it seems to me you can get back credentials from a dataset instance as well by doing my_pickle_dataset._redis_db.connection_pool.connection_kwargs.
This is a fair point! I don't have a good alternative at this point either and don't want to be a blocker to get this in. My suggestion is to merge this now as is and come back to it in case it becomes an issue. Thanks for your patience @gtauzin ! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merelcht Thank you :)
Signed-off-by: Guillaume Tauzin <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]> Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
|
It seems like the failing test is unrelated to the changes made in this PR. How should I address this? Thanks. |
You can ignore, unless you want to debug it for fun. Maybe the government shutdown took down the demo NASA data. 😜 |
This was just a random API timeout. All fine now, so I'm merging! 🎉 |
Description
While putting pipelines in production that rely on the
StudyDatasetthat I wrote in #1021, I noticed that providing it with a password incredentialsdoes not work. The password along with the database name, user name, host and ports are translated into a SQLAlchemy URL that was then casted to a string, resulting in the password becoming "***".Development notes
URLURL.render_as_string_study_name_existsmethod)In order to try out in a real case, you can make use of the
optuna_distbranch of kedro-dagster-example:Spin the postgresql DB:
In a separate terminal, do:
You can change from the local
StudyDataset(which is the one I pushed here as well -kedro_dagster_example.study_dataset.StudyDataset) to the one release inkedro-datasets(kedro_datasets_experimental.optuna.StudyDataset) in theconf/dev/catalog.ymlto observe the fix.Checklist
jsonschema/kedro-catalog-X.XX.jsonif necessaryRELEASE.mdfile